Skip to content

Retry a network source whose session dropped, not just failing media - #9

Open
owenpkent wants to merge 8 commits into
screenshot-real-volumesfrom
network-drop-retry
Open

owenpkent wants to merge 8 commits into
screenshot-real-volumesfrom
network-drop-retry

Conversation

@owenpkent

Copy link
Copy Markdown
Owner

Summary

Offloading from a NAS over a VPN, a large file failed outright. The engine already had everything needed to survive it — a transient read closes the dead handle, reopens it, seeks back to the last delivered chunk and carries on — but retry.py's classifier did not recognise a dropped SMB session as transient, so the retry never fired.

The specific gap: _TRANSIENT_WINERROR had 64 (ERROR_NETNAME_DELETED) and 121 (ERROR_SEM_TIMEOUT) but not 59 (ERROR_UNEXP_NET_ERR) — which is what Windows Explorer reports as 0x8007003B before abandoning an entire transfer. Same failure, same outcome.

Why these codes qualify

The module's thesis is that the discrimination matters more than the retrying, so each addition carries its reasoning: a read mid-copy proves the path resolved a moment earlier, so these are a session that died under us rather than a name that was never right. That is what separates them from ENOENT.

Added: 53, 54, 58, 59, 71, 1231, 1232, 1450 on Windows; ECONNRESET, ECONNABORTED, ENETRESET, ENETUNREACH, EHOSTUNREACH, ENOTCONN, EPIPE, ESTALE for a share mounted on Linux or macOS — only ever consulted there, since is_transient short-circuits on winerror, which Windows always sets.

Recovery costs one re-read of the chunk in flight. Nothing else changes: no new option, no behaviour change for a local card.

Context

The link itself was healthy — direct Tailscale path, 46 ms RTT, zero packet loss over 20 pings either side of the drop. MTU was already clamped to 1280 at the IP layer, and disabling SMB Multichannel made no difference. The fault is the session, not the data, which is why retrying is the right response.

Test plan

  • test_dropped_network_session_is_retried over all nine Windows codes
  • test_posix_network_mount_errors_are_retried for the errno path
  • Existing discrimination tests still pass (access denied, disk full, missing file stay permanent) — 52 in test_retry.py
  • Full suite green, ruff clean
  • Real offload from the NAS over the VPN: 22.8 MB verified, then re-verified against its MHL, checksums identical both sides

A read from an SMB share over a VPN dies mid-transfer and takes the open
handle with it, while the path stays reachable either side of the drop.
The machinery for that was already here — a transient read reopens the
handle and resumes from the last delivered chunk — but the classifier did
not count a dropped session as transient, so the retry never fired and the
file failed outright, exactly as Explorer does.

Add the network-mount codes, each with the reason it qualifies: mid-copy a
read proves the path resolved a moment ago, so these are a session that
died rather than a name that was never right. ERROR_UNEXP_NET_ERR is the
one Explorer surfaces as 0x8007003B before abandoning the transfer. The
POSIX equivalents go in too, for a share mounted on Linux or macOS, where
there is no winerror to short-circuit on.
The classifier short-circuits on winerror because it is the specific fact, and Python's errno mapping for a dropped session can land in the permanent set. Untested, that precedence could invert without any existing case noticing. Also pins the default: an unrecognised code is not retried, and Exhausted wrapping a network drop is not retried again at a coarser level.

@owenpkent owenpkent left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the added POSIX network errno values and Windows network error codes against the existing retry decision path. No additional actionable defect found in this diff.

Windows classification still gives winerror precedence over the mapped errno, and permanent missing-file/full-disk failures retain their existing behavior. This change expands which failures qualify for the existing retry machinery without altering source offsets, hash state, staging, or the verification verdict.

Validation: 56 targeted retry tests passed on this head, and git diff --check passed. These are injected-error tests, not evidence from a live SMB/VPN disconnect. The reopen-budget finding reported on parent PR #2 remains an inherited issue for this stack.

@owenpkent owenpkent left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8c30365 for transient-error classification and Windows/POSIX precedence.

No new finding in the added error-code handling. A specific Windows code still takes precedence over its generic errno, an unknown or permanent Windows code remains non-transient, and Exhausted still prevents the outer retry loop from granting a new budget to an exhausted chunk.

Validation: 19 focused error-classification regressions passed on this head. These are injected errors, not a live SMB/NFS disconnect. The read-recovery correction now present on #2 is absent from this branch's older parent snapshot, so carry that fix through the stack before relying on the expanded network-retry support.

@owenpkent owenpkent left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No additional actionable defect found in this incremental diff.

Reviewed the added network errno/winerror classifications, Windows error precedence, retryable versus permanent failures and integration with chunk recovery and Exhausted handling. The change retains immediate failure for permanent errors rather than making every source failure retryable.

Validation: retry.py and test_retry.py are byte-identical between this pinned head and detached descendant #12, where their checks passed within a 151-test focused stack run. The descendant engine was also inspected, so this result is distinguished from an independent checkout of #9. Current-head CI has successful runs, with older cancelled duplicates; the diff check passes.

Limits: injected network error codes, without a live SMB session drop, cable removal or server failover.

Pinned scope: declared base cdc4bc33a31626f1592777c36e7ad45ed9634e05, merge base cdc4bc33a31626f1592777c36e7ad45ed9634e05, head 8b0b202bc7ccf7efc620a86645261e9ba5caf944. Existing discussion and current-head checks were reviewed; no threads were resolved and no formal approval or merge is being issued.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant